-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Editor: Update BEM syntax to CSS modifer guidelines #19738
Conversation
@@ -91,9 +91,9 @@ describe( 'Basic rendering', () => { | |||
|
|||
const toggleState = container.querySelector( 'input[type="checkbox"]' ).checked; | |||
|
|||
const defaultControlGroup = container.querySelector( '.block-editor-responsive-block-control__group--default' ); | |||
const defaultControlGroup = container.querySelector( '.block-editor-responsive-block-control__group:not(.is-responsive)' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bummer that this requires the :not
pseudo-selector. I suppose it's not particularly an issue here, but selectors that might potentially have several modifiers will need extra care; scoping a selector for a component's "default" state could require multiple :not
checks that could get difficult to manage. Hopefully that's a relatively rare case though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(And I suppose it's more likely to be the case in tests like this; it's probably possible to architect without it being a concern in the CSS itself, and especially if some of the Emotion explorations move forward.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see this as mostly a use-case we'd want to isolate within tests, where we're explicitly wanting to distinguish between the default and the variant.
In real-world application, we should typically want to treat the variant as something of an additive layer upon the default, even to the extent of unsetting/overriding properties from those defaults, vs. ever feeling the need to use :not( .is-variant )
in CSS. If we found ourselves needing to use :not
, it probably raises a bigger question of whether we can really consider there to be a default (vs. other named variants, or treating them as wholly different elements).
288cd55
to
731a6a3
Compare
731a6a3
to
b4a46ad
Compare
Previously: #17846 (comment), #16790 (comment)
This pull request seeks to update incorrect usage of BEM modifier syntax to use the
is-
-based modifier syntax as recommended in the CSS naming guidelines.This impacts two components:
LinkControl
ResponveBlockControl
In the latter case, the implementation has been revised as mentioned at #16790 (comment) to unify the application of a single
is-responsive
modifier class.Open Question: There was a
hidden
attribute included previously inResponsiveBlockControl
which would never be applied given how the logic flowed. The changes here seek only to preserve the current behavior. If it needs to be applied, I can restore it as appropriate.Testing Instructions:
Repeat testing instructions from #17846 and #16790, verifying that there are no regressions.
Ensure tests pass: